-
Notifications
You must be signed in to change notification settings - Fork 3.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Building pblat for more platforms #51890
Conversation
📝 Walkthrough📝 WalkthroughWalkthroughThe pull request introduces modifications to the Possibly related PRs
Suggested labels
Suggested reviewers
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
🔇 Additional comments (5)recipes/pblat/pblat-aarch64.patch (1)
The removal of
The change makes the error handling more robust while maintaining security by keeping the essential Let's verify this fix works on the target platforms: ✅ Verification successfulLet me gather more information about the patch and its context. Correct fix for va_list handling in htmlVaBadRequestAbort The patch correctly removes the redundant The patch is:
🏁 Scripts executedThe following scripts were executed for the analysis: Script: #!/bin/bash
# Description: Check if this patch is being applied correctly in the build process
# and if there are any related platform-specific issues reported
# Check if the patch is referenced in meta.yaml
rg "pblat-aarch64.patch" -A 5
# Look for any related platform-specific issues
gh issue list --repo bioconda/bioconda-recipes --search "pblat aarch64" --state all
Length of output: 381 Script: #!/bin/bash
# Check the full patch content to understand the context
cat recipes/pblat/pblat-aarch64.patch
# Check if there are any other patches for pblat
fd ".*\.patch$" recipes/pblat/
# Look for any build logs or test failures
rg "pblat.*aarch64" -A 5
Length of output: 810 recipes/pblat/meta.yaml (4)
Line range hint The recipe maintains all required sections and properly preserves metadata while adding multi-platform support. The changes are focused and align well with the PR objectives for expanding platform support.
The patch is conditionally applied for aarch64 architecture, but we should verify its contents and implementation. ✅ Verification successfulPatch file verified and appears correct for aarch64 support The patch modifies the
🏁 Scripts executedThe following scripts were executed for the analysis: Script: #!/bin/bash
# Description: Check if the patch file exists and examine its contents
# Expected: Find and display the patch file contents
# Look for the patch file
fd "pblat-aarch64.patch" recipes/pblat
# If found, display its contents
if [ $? -eq 0 ]; then
echo "=== Patch contents ==="
cat recipes/pblat/pblat-aarch64.patch
fi
Length of output: 596
The addition of linux-aarch64 and osx-arm64 platforms is aligned with the PR objectives. However, we should verify:
✅ Verification successfulBased on the search results, I can now generate the final response as we have sufficient evidence about platform support in the bioconda ecosystem. Platform support for linux-aarch64 and osx-arm64 is well-established The search results show multiple bioconda packages successfully supporting these platforms through:
This demonstrates established patterns and infrastructure support for both linux-aarch64 and osx-arm64 platforms in the bioconda ecosystem, including the presence of a platform-specific patch for pblat itself. 🏁 Scripts executedThe following scripts were executed for the analysis: Script: #!/bin/bash
# Description: Check for platform-specific configurations and dependencies
# Expected: Find any platform-specific build or dependency configurations
# Look for platform-specific configurations in other bioconda recipes
echo "=== Checking for platform-specific patterns in other recipes ==="
rg -A 5 "(\[aarch64\]|\[arm64\])" recipes/
# Check for any build configuration files
echo "=== Checking for build configuration files ==="
fd "conda_build_config|config.yaml" .
Length of output: 12203 Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
recipes/pblat/meta.yaml (1)
Line range hint
14-24
: Consider adding run_exports for dependency version locking.To prevent potential ABI breakage, consider adding a
run_exports
section for the C library dependencies. This ensures downstream recipes are built against compatible versions.Example addition:
requirements: build: - make - {{ compiler('c') }} host: - zlib - openssl run: - zlib - openssl run_exports: - {{ pin_subpackage('pblat', max_pin='x.x.x') }}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
recipes/pblat/meta.yaml
(2 hunks)
🔇 Additional comments (2)
recipes/pblat/meta.yaml (2)
12-12
: LGTM: Build number increment is appropriate.
The build number increment is correct for adding platform support without changing the package version.
37-39
: Verify build compatibility for new architectures.
While adding support for linux-aarch64
and osx-arm64
is valuable, please ensure:
- The C code compiles correctly on these architectures
- All dependencies are available for these platforms
- Consider adding platform-specific build flags if needed
Let's verify the availability of dependencies on these platforms:
Signed-off-by: Martin Tzvetanov Grigorov <[email protected]>
@BiocondaBot please fetch artifacts |
Package(s) built are ready for inspection:
|
LGTM! |
@BiocondaBot please add label |
Describe your pull request here
Please read the guidelines for Bioconda recipes before opening a pull request (PR).
General instructions
@BiocondaBot please add label
command.@bioconda/core
in a comment.Instructions for avoiding API, ABI, and CLI breakage issues
Conda is able to record and lock (a.k.a. pin) dependency versions used at build time of other recipes.
This way, one can avoid that expectations of a downstream recipe with regards to API, ABI, or CLI are violated by later changes in the recipe.
If not already present in the meta.yaml, make sure to specify
run_exports
(see here for the rationale and comprehensive explanation).Add a
run_exports
section like this:with
...
being one of:{{ pin_subpackage("myrecipe", max_pin="x") }}
{{ pin_subpackage("myrecipe", max_pin="x.x") }}
{{ pin_subpackage("myrecipe", max_pin="x.x") }}
(in such a case, please add a note that shortly mentions your evidence for that){{ pin_subpackage("myrecipe", max_pin="x.x.x") }}
(in such a case, please add a note that shortly mentions your evidence for that){{ pin_subpackage("myrecipe", max_pin=None) }}
while replacing
"myrecipe"
with eithername
if aname|lower
variable is defined in your recipe or with the lowercase name of the package in quotes.Bot commands for PR management
Please use the following BiocondaBot commands:
Everyone has access to the following BiocondaBot commands, which can be given in a comment:
@BiocondaBot please update
@BiocondaBot please add label
please review & merge
label.@BiocondaBot please fetch artifacts
You can use this to test packages locally.
Note that the
@BiocondaBot please merge
command is now depreciated. Please just squash and merge instead.Also, the bot watches for comments from non-members that include
@bioconda/<team>
and will automatically re-post them to notify the addressed<team>
.